Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add stripslashes to mssql result columns #13534

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Add stripslashes to mssql result columns #13534

merged 1 commit into from
Jan 11, 2017

Conversation

csthomas
Copy link
Contributor

@csthomas csthomas commented Jan 9, 2017 •
edited by jeckodevelopment
Loading

Pull Request for Issue #13507 .

Summary of Changes

SQL Server database always escape backslash in result.
This PR stripslashes for each column from result query.

Testing Instructions

TEST 1

Before patch

  1. Go to article, change some text or nothing, save article a few times.
  2. Go to "Versions" (button at the top), there should open a modal window with list of dates and versions.
  3. Click on some date row which open popup window.
  4. You should see warnings and no data information about changes between versions.
Warning: Invalid argument supplied for foreach() in .../administrator/components/com_contenthistory/helpers/contenthistory.php on line 32

Warning: Invalid argument supplied for foreach() in .../administrator/components/com_contenthistory/helpers/contenthistory.php on line 295

After patch the warning does not appears and you see some information.

TEST 2

Before patch

  1. Edit new article, and write two chars in full text: \"
  2. Re-save a few times without changing anything more.
  3. You should see more and more backslashes, ignore html tags in ex. \\", next \\\\", etc.

After patch the number of backslashes are not increased on each save.

TEST 3

Before patch:

  1. Edit article and add to Link A Text (Images and Links Tab): A""B.
  2. After you save article you get Error decoding JSON data: Syntax error.

After patch

  1. Do the same and save and load article works as expected.

Documentation Changes Required

N/A

@RonakParmar
Copy link

Missing testing instruction.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@zero-24 zero-24 mentioned this pull request Jan 10, 2017
Closed
@csthomas
Copy link
Contributor Author

Test instruction added.

There is a lots of problems with mssql. This PR only fix a few of them.
There is a lots of places where something does not work this not means this PR is buggy.

"This PR fix only SELECT queries".

@waader
Copy link
Contributor

waader commented Jan 10, 2017

I have tested this item ✅ successfully on f57b5ff

Thanks!

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@alikon
Copy link
Contributor

alikon commented Jan 11, 2017

I have tested this item ✅ successfully on f57b5ff

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@jeckodevelopment
Copy link
Member

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 11, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Jan 11, 2017
@rdeutz
Copy link
Contributor

rdeutz commented Jan 11, 2017

Just one question, you removed loadResult from the class because the JDatabaseDriver class does the job ok?

@csthomas
Copy link
Contributor Author

yes

@rdeutz rdeutz merged commit e3d8732 into joomla:staging Jan 11, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 11, 2017
@csthomas csthomas deleted the fixmssql2 branch January 11, 2017 19:45
@dgrammatiko
Copy link
Contributor

@rdeutz isn't the removal of a public function a b/c break?

@csthomas
Copy link
Contributor Author

It is not removed because it is inherited from JDatabaseDriver

@dgrammatiko
Copy link
Contributor

@csthomas in such case ignore my comment

andrepereiradasilva pushed a commit to andrepereiradasilva/joomla-cms that referenced this pull request Jan 12, 2017
@csthomas @andrepereiradasilva
Add stripslashes to mssql result columns. (joomla#13534)
725755d
roland-d added a commit to roland-d/joomla-cms that referenced this pull request Jan 13, 2017
@roland-d
Merge branch 'staging' of github.com:roland-d/joomla-cms into trackas…
aa225cb 
…sets-option

* 'staging' of github.com:roland-d/joomla-cms: (136 commits)
  Clean up old code in cache.php file (joomla#12183)
  Fixing search for MySQL (joomla#13571)
  Unnecessary double quotes in  /libraries/joomla (joomla#13372)
  Some improvements in tests #3: (joomla#13402)
  It's 2017. Happy New Year
  Fixing a typo in gallery plugin language files
  Add stripslashes to mssql result columns. (joomla#13534)
  remove unneeded space from btn-group/radio/checkboxes (joomla#12003)
  typo (joomla#13563)
  Remove default value from the field params to inherit from plugin
  Remove multiple parameter from user field
  Fix name of component helper in fieldshelper (joomla#13539)
  remove duplicated code (joomla#13550)
  Fix invalid string that causes the ini file not to load (joomla#13544)
  Catch "expects parameter 2 to be string" error
  Take complete context for group lookup (joomla#13538)
  [Mssql] Fix syntax error when installing a language in backend (joomla#13512)
  Normalize #__categories table across 3 db systems and add default values (joomla#13514)
  Normalize #__ucm_content table across 3 db systems and add default values (joomla#13513)
  Update config.xml (joomla#13503)
  ...
@csthomas csthomas mentioned this pull request Jan 13, 2017
Merged
@csthomas
Copy link
Contributor Author

csthomas commented Jan 14, 2017 •
edited
Loading

After more deep searching I have found this PR fixes errors but does not do it in the right way.
It was weird to me that I have to use stripslashes() but it worked.

At the end I have found that $db->quote() does not work correctly and generate additional backslashes that are saved to database (specially to columns with encoded json). After that stripslashes() is needed.

After I changed $db->quote() code then stripslashes() is not needed.
Take a look at PR #13585

@csthomas csthomas mentioned this pull request Jan 16, 2017
Merged
rdeutz pushed a commit to rdeutz/joomla-cms that referenced this pull request Jan 17, 2017
@csthomas @rdeutz
Add stripslashes to mssql result columns. (joomla#13534)
eae7293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Joomla 3.7.0
Development

Successfully merging this pull request may close these issues.
8 participants
@csthomas @RonakParmar @waader @alikon @jeckodevelopment @rdeutz @dgrammatiko @joomla-cms-bot